Skip to content

feat: Add Subtitle Offset Setting to Android TV App - #108

Closed
EierKopZA wants to merge 3 commits into
ProdigyV21:mainfrom
EierKopZA:feature/subtitle-offset
Closed

feat: Add Subtitle Offset Setting to Android TV App#108
EierKopZA wants to merge 3 commits into
ProdigyV21:mainfrom
EierKopZA:feature/subtitle-offset

Conversation

@EierKopZA

Copy link
Copy Markdown
Contributor

Description

This pull request adds a new quality-of-life feature allowing users to vertically offset their subtitles on the screen during playback.

Some users prefer subtitles sitting lower out of the active content edge, or slightly higher to match external borders. To accommodate this, a 5-step configurable "Subtitle Offset" setting has been added alongside existing options (Subtitle Color, Subtitle Size).

Changes Made:

  • Added subtitleOffset DataStore preference mapping in SettingsViewModel and PlayerViewModel.
  • Embedded SettingsRow within GeneralSettings section inside the SettingsScreen UI layer.
  • Subtitle views are configured with setBottomPaddingFraction within PlayerScreen.kt, dynamically reacting to the offset fractions (0.02f to 0.18f) requested.

No existing settings routines or ExoPlayer dependencies were modified or broken.

@Himanth-reddy

Copy link
Copy Markdown
Collaborator

Great feature addition. The subtitle offset setting is a useful quality-of-life improvement and fits naturally with the existing subtitle controls.

One small technical note: ensure the subtitle offset is applied in both the player view initialization and the update path, so changes take effect immediately during playback without needing to recreate the player view.

@Himanth-reddy Himanth-reddy self-assigned this Apr 3, 2026
@ProdigyV21

Copy link
Copy Markdown
Owner

Thanks for this PR — the subtitle offset feature itself looks good and CI was green. However, after merging #120 (UI Mode Warning Dialog), this branch now conflicts with main in SettingsScreen.kt. It's now in a CONFLICTING / DIRTY state and can't be merged as-is.

To unblock merge, please:

  1. Rebase on latest main and resolve the conflicts in app/src/main/kotlin/com/arflix/tv/ui/screens/settings/SettingsScreen.kt. The conflicts will be in the General settings section where Add UI Mode Warning Dialog and Handle Device Mode Changes #120 added UI mode confirmation handling.
  2. After rebasing, double-check the focus indices in GeneralSettings. Inserting your Subtitle Offset row shifts every item after it down by one, so any focusedIndex == N checks and the max-index navigation clamp (look for a comment like 0 -> N // General: N items) need to be updated to keep DNS Provider and every row below the new one reachable via D-pad. This has been a recurring issue in other recent PRs that added settings rows (feat: add toggle for trailer banner audio #110, feat: implement episode spoiler protection #112), so worth verifying explicitly.
  3. (Optional, from the earlier review) Apply the subtitle offset in the PlayerView update path in addition to initialization, so changes take effect live during playback without recreating the view.

Once rebased with correct focus indices, this should be ready to merge.

ProdigyV21 pushed a commit that referenced this pull request Apr 5, 2026
Reported: "On Fire Stick the remote button rewind/ffwd/play/pause
buttons don't work with the player."

Root cause: Only `Key.MediaPlayPause` / `MediaPlay` / `MediaPause` was
handled in PlayerScreen.kt, and only inside the `if (showSubtitleMenu)`
branch \u2014 so media keys did nothing unless the user had already opened
the audio/subtitle menu. `Key.MediaRewind`, `MediaFastForward`,
`MediaNext`, `MediaPrevious`, and `MediaStop` were not handled at all,
anywhere.

Fire TV Stick remotes ship with dedicated FF/RW/Play buttons and Amazon
explicitly recommends apps handle them via the standard KeyEvent
constants. Bluetooth A2DP headsets and keyboards also produce these
keys. ARVIO was dropping all of them on the floor.

Fix: add a top-level media-key dispatch at the very start of the player
`.onKeyEvent` handler, BEFORE the error / menu / overlay branches. Each
key returns `true` immediately so it always wins regardless of whether
a menu is open. Handles:

- `Key.MediaPlayPause` \u2192 toggle play/pause, show controls.
- `Key.MediaPlay` \u2192 play, show controls.
- `Key.MediaPause` \u2192 pause, show controls.
- `Key.MediaStop` \u2192 pause and exit the player (equivalent to Back).
- `Key.MediaRewind` \u2192 seek back 10 seconds via existing queueControlsSeek.
- `Key.MediaFastForward` \u2192 seek forward 10 seconds.
- `Key.MediaNext` \u2192 jump to the next episode (TV only). Uses the same
  onPlayNext lambda the existing Next Episode button uses, so the
  binge-group / source-preservation logic is identical.
- `Key.MediaPrevious` \u2192 jump to the previous episode (TV only, guarded
  by `episodeNumber > 1`).

The existing `Key.MediaPlayPause` branch inside the subtitle menu
remains in place as a harmless safety net \u2014 it becomes unreachable in
practice because the top-level dispatch already returns true, but
leaving it means a future change to the top handler can't accidentally
break media keys inside the subtitle menu.

Closes the Fire TV remote part of #68. The other parts of #68
(subtitle timing sync, in-player episode picker) are scoped
separately. Subtitle offset is already being shipped in PR #108.
ProdigyV21 added a commit that referenced this pull request Apr 5, 2026
#133)

Reported: "On Fire Stick the remote button rewind/ffwd/play/pause
buttons don't work with the player."

Root cause: Only `Key.MediaPlayPause` / `MediaPlay` / `MediaPause` was
handled in PlayerScreen.kt, and only inside the `if (showSubtitleMenu)`
branch \u2014 so media keys did nothing unless the user had already opened
the audio/subtitle menu. `Key.MediaRewind`, `MediaFastForward`,
`MediaNext`, `MediaPrevious`, and `MediaStop` were not handled at all,
anywhere.

Fire TV Stick remotes ship with dedicated FF/RW/Play buttons and Amazon
explicitly recommends apps handle them via the standard KeyEvent
constants. Bluetooth A2DP headsets and keyboards also produce these
keys. ARVIO was dropping all of them on the floor.

Fix: add a top-level media-key dispatch at the very start of the player
`.onKeyEvent` handler, BEFORE the error / menu / overlay branches. Each
key returns `true` immediately so it always wins regardless of whether
a menu is open. Handles:

- `Key.MediaPlayPause` \u2192 toggle play/pause, show controls.
- `Key.MediaPlay` \u2192 play, show controls.
- `Key.MediaPause` \u2192 pause, show controls.
- `Key.MediaStop` \u2192 pause and exit the player (equivalent to Back).
- `Key.MediaRewind` \u2192 seek back 10 seconds via existing queueControlsSeek.
- `Key.MediaFastForward` \u2192 seek forward 10 seconds.
- `Key.MediaNext` \u2192 jump to the next episode (TV only). Uses the same
  onPlayNext lambda the existing Next Episode button uses, so the
  binge-group / source-preservation logic is identical.
- `Key.MediaPrevious` \u2192 jump to the previous episode (TV only, guarded
  by `episodeNumber > 1`).

The existing `Key.MediaPlayPause` branch inside the subtitle menu
remains in place as a harmless safety net \u2014 it becomes unreachable in
practice because the top-level dispatch already returns true, but
leaving it means a future change to the top handler can't accidentally
break media keys inside the subtitle menu.

Closes the Fire TV remote part of #68. The other parts of #68
(subtitle timing sync, in-player episode picker) are scoped
separately. Subtitle offset is already being shipped in PR #108.

Co-authored-by: Arvin <arvin@arflix.local>
@EierKopZA EierKopZA closed this May 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants